Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Combined robot hw kinetic #235

Merged

Conversation

toliver
Copy link
Contributor

@toliver toliver commented May 6, 2016

This PR has been reopen against kinetic-devel as suggested in #234 (comment).

To see a description of the contents and the discussion history please have a look at #231 and #220.

This will make it possible to combine several RobotHW objects into a single one.
…robot_hw allows to load different RobotHW as plugins and combines them into a single RobotHW. A single controller manager can then access resources from any robot.
@bmagyar
Copy link
Member

bmagyar commented May 13, 2016

@paulbovbel @mikepurvis @efernandez isn't this interesting for you guys?
@davetcoleman any thoughts?
Thanks Toni, I'll do some testing and get back here.

@davetcoleman
Copy link
Member

LGTM +1

@bmagyar
Copy link
Member

bmagyar commented May 22, 2016

I've done several passes over it and played with the tests too, seems all good. I'm going to merge it now and quickly release for first Kinetic sync 👍

@k-okada
Copy link

k-okada commented Oct 21, 2017

Hi @toliver, @bmagyar, @adolfo-rt .....

Since this patch, we can not compile pr2_controller_manager as reported at PR2/pr2_mechanism#330

/home/yisha/workspace/gsoc/catkin_ws/src/pr2/pr2_mechanism/pr2_controller_manager/test/controllers/test_controller.cpp:131:61:   required from here
/opt/ros/kinetic/include/hardware_interface/internal/interface_manager.h:194:21: error: no matching function for call to ‘pr2_mechanism_model::RobotState::RobotState()’
         iface_combo = new T;

I'm not sure what is the root cause of this, but just want someone to confirm adding empty constructor is ok or not.

 class RobotState : public hardware_interface::HardwareInterface
  {
  public:
    /// constructor
 +  RobotState();
    RobotState(Robot *model);

c.f. k-okada/pr2_mechanism@13af6b9.

@mathias-luedtke
Copy link
Contributor

Obviously, this PR introduced a break by requiring a default constructor.
At a first look there might be a way to remove this new requirement by extendeding the SFINAE workaround. I will try that and fix the warnings this PR introduced ;)

but just want someone to confirm adding empty constructor is ok or not.

All interfaces in hardware_interface come with a default constructor, but they check for this case of empty pointers and handle it. However, I don't see real need for them to have these (this depends on the actual RobotHW implementation).

In the case of your RobotState class I would consider a default constructor a problem, because model_ is public. In order to have an empty constructor you should make in private, add a getter if needed and harden all ponter accesses.

@mikepurvis
Copy link
Contributor

mikepurvis commented Nov 8, 2017

I finally gave this a shot in earnest, and couldn't believe how seamless it was. Big props to everyone involved for making it happen— really nice being able to compose a robot from RobotHW plugins using just a launch file and parameters. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants